Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added lazy loading to merge books #9312

Conversation

Realmbird
Copy link
Contributor

Closes #9201

Add lazy loading for covers in works merge tool

Technical

Just adds loading lazy to images for books in merge

Testing

  1. docker compose up
  2. docker compose run --rm home npm run build-assets
  3. docker compose exec -e PYTHONPATH=. web bash -c "./scripts/copydocs.py /works/OL21868175W /works/OL41495W"
  4. http://localhost:8080/works/merge?records=OL21868175W,OL41495W
  5. Inspect
  6. Look at number of requests
  7. Observe how number of requests stops around 180

Screenshot

Screenshot from 2024-05-22 09-03-23
Screenshot from 2024-05-22 09-05-50

Stakeholders

@cdrini @RayBB

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and working.
Though, in my experience is is loading 135 covers vs 167.
So I think maybe there is something with the way Vue renders this that causes many covers to still load. But I think this small improvement should the default anyway.

@cdrini can you review/merge?

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label May 22, 2024
@cdrini
Copy link
Collaborator

cdrini commented May 23, 2024

Ah interesting ; try adding a min-height: 80px to the img tag's css. My guess would be at the beginning, since no height is specified, all the images have a height of effectively 1px. This means they are all on screen, causing the loading=lazy to have no effect. Also add a comment describing why the min-height was added to the CSS; that'll help future folks!

@cdrini cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels May 23, 2024
@Realmbird Realmbird force-pushed the 9201/fix/lazy-loading-covers-merge-tool branch from 7586519 to 3117cf6 Compare May 31, 2024 19:27
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label May 31, 2024
@Realmbird
Copy link
Contributor Author

@cdrini @RayBB Added minimum height

@RayBB
Copy link
Collaborator

RayBB commented Jun 3, 2024

@Realmbird in your testing did adding that height effect anything?
As far as I can tell, it didn't.
@cdrini from my research, since we aren't rendering the Vue serverside, the lazy-loading doesn't work as expected since the html is created by vue after the page is loaded. That's why there are libraries like https://github.com/alexjoverm/v-lazy-image

So this PR has a mild effect but doesn't quite do what we want and it looks like that's not simple to do.
I propose
1: We merge it with lazy loading as is for the mild gain
2. Look into this again after we upgrade to Vue 3 and maybe considering doing some server side rendering to avoid this issue (I don't know if we can really do that but we could investigate then).

@cdrini
Copy link
Collaborator

cdrini commented Jun 3, 2024

Ah my bad, I just realized this was for the edition snippet! The edition snippet is working with loading="lazy" 👍 You can see it working as you scroll, and images load in. The fact that it's client-side shouldn't affect this ; I've used loading="lazy" with Vue many times and it works quite well.

It's the work covers that need to have loading="lazy" also added, and, potentially, need a min-height.

These:

image

So you can:

  1. remove the min-height here
  2. add loading="lazy" to the above images, and, if needed, add min-height to those images.

@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jun 3, 2024
@RayBB
Copy link
Collaborator

RayBB commented Sep 3, 2024

@Realmbird do you think you could incorporate drini's feedback?

@RayBB
Copy link
Collaborator

RayBB commented Nov 23, 2024

@Realmbird still interested in working on this one?

@Realmbird
Copy link
Contributor Author

No sorry forgot to unassign

@RayBB
Copy link
Collaborator

RayBB commented Nov 26, 2024

Closing this so someone else can work on it.
Thanks for your efforts

@RayBB RayBB closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lazy loading for covers in works merge tool
3 participants